This adds a dependency on the objc2 ecosystem (objc2, block2 and objc2-encode), which I maintain. They are fairly widely used, and Mozilla gfx-rs/wgpu#5641, though I acknowledge that this could still be seen as a supply-chain attack. Implementing the execution policy check without these crates is doable, though a lot more code.

What should the config option be named? I went with build.detect-antivirus, an alternative would be to put it under [term]. Or maybe build.detect-slow-bin-first-run, to focus on the intention instead of what it does?

Another option is we move forward with an [advice] table, mirroring git's. It would be ideal if we could suggest a command to run to turn off the advice (rather than saying how to edit a file)

I know [advice] has at least come up at #15887 (comment), unsure where else.

An entry in the [advice] table could also disable the note introduced in #13371.

This needs further work on the wording!

Previously discussed in #15908 (comment).

I went with "session" over "terminal", since this term also applies if you're connecting via ssh (or running Cargo directly as a launchd agent). WDYT?

(Technically, I think it's something like "process context" or "TCC entitlement inheritance state", but the other terms are probably clearer).

Actually, never mind, "terminal" is probably clearer for the user, so instead I've used "session" at the beginning of the diagnostic, and "terminal" later.

Current wording:

note: detected that XProtect is enabled in this session, which may slow down builds as it scans build scripts and test binaries before they are run.
If you trust the software that you run in your terminal, then this overhead can be avoided by giving it more permissions under `System Preferences > Security & Privacy > Developer Tools`. (Cargo has made an entry for the current terminal appear there, though you will need to go and manually enable it).
Alternatively, you can disable this note by adding `build.detect-antivirus = false` to your ~/.cargo/config.toml.
See <https://doc.rust-lang.org/cargo/appendix/antivirus.html#xprotect> for more information.

I am a bit unsure whether the end of create_bcx is the right place for this check?

Any particular reason for the concern?

Note that we have another type of check in here, the rustc compatibility one which you put this right after so that works from a consolidating environment checks perspective.

I think it's the right place, but I just wanted to call to attention that I'm not familiar with Cargo's internals, and that there might be a more suited place? But if you say it's correct, then it's correct.

If we want this heuristic, what should the threshold be?

As a data point, a simple binary using reqwest (in blocking mode) currently requires 10 build scripts.

One challenge with this heuristic is that tests, doctests, and build scripts are very different

For tests (in cargo tests model), the overhead of the test itself is likely to dwarf XProtect. We don't have parallel test running yet (working on the foundation to allow it) which defers that problem.

For doctests, 2024 edition at least reduces the number of binaries. However, there is no caching of the binaries that are built (yet) so there are no binaries on every run which is a problem if there is any memory to XProtect.

Both of the above may not be relevant if the user is doing a cargo check or even a cargo build which is what this location is for.

Regarding build scripts, I wouldn't expect those in a beginner project except through dependencies which can really blow up the numbers with deps like reqwest atm. After #14948, beginnings should hopefully only deal with build scripts for -sys packages and I have a likely unrealistic dream that we can consolidate those down to 1. Larger projects may also have build scripts for gathering commit information and adding a windows manifest.

For build scripts, the single threaded nature of XProtect becomes a problem for build scripts that are likely to run in parallel.

My counter proposals

• no binaries check
• only check build scripts for now and note in cargo test --all should run tests in parallel #5609 that we should add a check like this to cargo test, and either
  • check if any build scripts (0<num_binaries)
  • check a small number (e.g. 4<num_binaries)

One challenge with this heuristic is that tests, doctests, and build scripts are very different

Just throwing an idea out there, we could make it a weighted sum instead?

let binary_weight = unit_graph.keys().filter(|unit| match unit.mode {
    UnitMode::RunCustomBuild => 1.0,
    UnitMode::Test => 0.5, // Or something
    UnitMode::Doctest => 0.7, // Or something
    _ => 0.0,
}).sum();

For tests (in cargo tests model), the overhead of the test itself is likely to dwarf XProtect. We don't have parallel test running yet (working on the foundation to allow it) which defers that problem.

Not sure I agree with this assessment? If you have a lot of integration tests, it's going to take a long time to run all those if XProtect wants to scan them all first. Sure, there's also build and link time of those, but that can be done in parallel.

A quick test with 100 integration tests revealed ~2 seconds build+link time and ~15 seconds test run time on my machine. With XProtect disabled, the test run time was down to ~0.4 seconds (even though they were run serially by Cargo).

For doctests, 2024 edition at least reduces the number of binaries. However, there is no caching of the binaries that are built (yet) so there are no binaries on every run which is a problem if there is any memory to XProtect.

I'm not sure I understand what you mean? If doctest's binaries aren't cached, then there's even more reason why XProtect is going to be problematic there?

Both of the above may not be relevant if the user is doing a cargo check or even a cargo build which is what this location is for.

True. Not that a plain cargo check/cargo build will produce UnitMode::Test, but cargo test --no-run and cargo build --tests both do, and in those cases the heuristic is definitely wrong.

Maybe there's a way to know how many binaries we're actually going to run? Or maybe we should do the check later?

Regarding build scripts, I wouldn't expect those in a beginner project except through dependencies which can really blow up the numbers with deps like reqwest atm.

Agreed, and yes, we should also work on reducing that.

A quick test with 100 integration tests revealed ~2 seconds build+link time and ~15 seconds test run time on my machine. With XProtect disabled, the test run time was down to ~0.4 seconds (even though they were run serially by Cargo).

Uh, wow. I had assumed that the biggest problem was that it runs serially so the overrhead for an already serial operation would be minimal. This is strange.

I'm not sure I understand what you mean? If doctest's binaries aren't cached, then there's even more reason why XProtect is going to be problematic there?

Yes, thats what I'm calling out. On the surface level, Edition 2024 reduced the impact of this but the fact that they are rebuilt every run is a problem.

True. Not that a plain cargo check/cargo build will produce UnitMode::Test, but cargo test --no-run and cargo build --tests both do, and in those cases the heuristic is definitely wrong.

That builds them but still doesn't run them and so therefore isn't running into the issue. I feel weird providing the message speculatively like that which is why I was suggesting the test check be in cargo test. Granted, I thought that wouldn't be a problem until #5609 but maybe it would be needed now.

One thing I wanted to note is that there is a performance cost to this. At least on my system, an error result took over 60ms, and a passing result took 14ms. We try to keep cargo's overhead relatively small (I like to keep the budget under 500ms total). 14ms is probably ok, but it's inching towards the territory where it is significant.

Hmm, you're right.

On my machine (to have comparable numbers to those I've given elsewhere), I get:

This is of course unfortunate, and is more reason to not do the check unless the heuristic triggers (whatever the heuristic ends up being).

Messaging-wise, this seems much more widespread than just trusting cargo packages and dependencies, since it is authorizing the terminal/editor/tool that cargo is running under. That means, for example, you have to trust everything that happens in the terminal or editor.

One question that came up is whether or not it is possible to trust just cargo/rustup, or does it have to be the App that it is running under? It wasn't really clear to us what the rules were here.

One question that came up is whether or not it is possible to trust just cargo/rustup, or does it have to be the App that it is running under? It wasn't really clear to us what the rules were here.

If there is, I haven't found a way to do so. Adding just the cargo / rustup binary as a Developer Tool does not work.

I recall someone suggesting something about making a launchd daemon (or agent). I think that would allow cargo to XPC to cargod and request it to build and run build scripts, and only have the daemon have the elevated privileges - but there's wayyy too many issues with that idea.

Messaging-wise, this seems much more widespread than just trusting cargo packages and dependencies, since it is authorizing the terminal/editor/tool that cargo is running under. That means, for example, you have to trust everything that happens in the terminal or editor.

I have tried to make the text a bit clearer in this regard, the current wording is:

note: detected that XProtect is enabled in this session, which may slow down builds as it scans build scripts and test binaries before they are run.
If you trust the software that you run in your terminal, then this overhead can be avoided by giving it more permissions under `System Preferences > Security & Privacy > Developer Tools`. (Cargo has made an entry for the current terminal appear there, though you will need to go and manually enable it).
Alternatively, you can disable this note by adding `build.detect-antivirus = false` to your ~/.cargo/config.toml.
See <https://doc.rust-lang.org/cargo/appendix/antivirus.html#xprotect> for more information.

See also appendix/antivirus.md that I just added to the docs.

One question that came up is whether or not it is possible to trust just cargo/rustup, or does it have to be the App that it is running under? It wasn't really clear to us what the rules were here.

Like TCC permissions, the developer tool exception applies to the "responsible process" which would usually be the terminal emulator application (iTerm/Terminal.app/etc). Targeting cargo only would require using this macOS API to set a given child process as responsible for itself (and all it's children):

int responsibility_spawnattrs_setdisclaim(posix_spawnattr_t attrs, int disclaim);

See https://www.qt.io/blog/the-curious-case-of-the-responsible-process for more info

Thanks for the link!

Notably, if we wanted the user to give Cargo those permissions, it would not be something that Cargo could easily do. It has to be the terminal itself that does that.

Unless maybe if Cargo were to spawn a child copy of itself? Or maybe we could get the Rustup wrappers to do it? That said, it'd probably only shift and not eliminate the attack vector, if we were to do this then Cargo could still be spawned by a malicious actor to obtain the heightened privileges.

I see there may be some discussion about the exact configuration value, but I wanted to note that this currently isn't correct since false isn't a valid value. Just want to make sure that all gets updated.

I intended for:

• -Zdetect-antivirus=auto, -Zdetect-antivirus=always and -Zdetect-antivirus=never, because I thought that would be useful while testing.
• [build] detect-antivirus = false or [build] detect-antivirus = true in configuration file, I don't actually want users to be able to tweak the heuristic of when this fires.

What do you mean by false not being a valid value?

I added this appendix to the documentation, so that the note can link to it. Needs iteration on the wording.

As an alternative, or additional thing, we could consider only doing the check once every X days, a bit similar to the automatic garbage collection.

This has the problem that it might take a long time for the user to notice the message, and worse, it makes Cargo's output non-deterministic, so I'm inclined not to do it.